fix: preserve identifier quoting in CHECK constraints and fix BETWEEN parsing#11
Conversation
… parsing This PR fixes two related issues with CHECK constraint parsing: 1. CHECK constraints with camelCase columns were losing their quotes - Added minimal quoteIdentifierIfNeeded() to preserve camelCase quoting - Modified extractColumnName() to quote identifiers when needed 2. BETWEEN expressions were incorrectly parsed as BETWEEN (X, Y) instead of BETWEEN X AND Y - Fixed parseAExpr() to handle BETWEEN with proper AND syntax - Removed unnecessary parentheses around simple comparisons These changes ensure that CHECK constraints work correctly with camelCase column names and complex expressions, which is critical for compatibility with modern ORMs like Better Auth and Prisma. Note: Aware of minor code duplication in quoting logic; planning to extract to shared util package in follow-up PR to keep this fix focused and minimal. Fixes identifier quoting issues reported with Better Auth integration.
There was a problem hiding this comment.
Pull Request Overview
This PR fixes two critical issues with CHECK constraint parsing that affect schemas using camelCase naming conventions: preserving identifier quoting and correcting BETWEEN expression parsing.
- Added identifier quoting preservation to maintain camelCase column names in CHECK constraints
- Fixed BETWEEN parsing to use proper
X AND Ysyntax instead of incorrect tuple syntax - Added defensive parentheses handling for CHECK clause generation
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| internal/ir/parser.go | Added identifier quoting logic and fixed BETWEEN parsing in expression handling |
| internal/diff/table.go | Added defensive CHECK clause parentheses normalization in SQL generation |
| internal/diff/identifier_quote_test.go | Added comprehensive test cases for CHECK constraint quoting scenarios |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
tianzhou
left a comment
tianzhou
left a comment
There was a problem hiding this comment.
Thanks for the PR, could you please
- Resolve the conflict (due to another PR that you authored)
- Run the full test to make sure we don't break anything "go test -v ./..."
|
I have to admit that I haven't thought about this identifier case issue in the first place. Do you have a minimal example that I can follow to reproduce the issue? I would like to add a comprehensive test case. |
|
Sorry, I'm bit busy. Hopefully tomorrow I can work on this. All I was trying to do was to use Better Auth library with SvelteKit project + pgschema. Better Auth has CLI tool to generate schema and uses camelCase. That's why I ended up needing to handle mixed case identifiers. Here's a minimal example to reproduce the issue: -- Better Auth generates tables with camelCase
CREATE TABLE "user" (
"id" TEXT PRIMARY KEY,
"email" TEXT UNIQUE,
"emailVerified" BOOLEAN
);
CREATE TABLE "session" (
"id" TEXT PRIMARY KEY,
"userId" TEXT REFERENCES "user"("id"),
"expiresAt" TIMESTAMP
);
-- The issue appears when CHECK constraints reference these columns
ALTER TABLE "user"
ADD CONSTRAINT "email_check"
CHECK ("emailVerified" IS NOT NULL);The problem is that pgschema doesn't preserve the quotes around camelCase identifiers in CHECK constraints. Without quotes, PostgreSQL converts them to lowercase and then can't find the column. Expected output should preserve quotes: Not: Let me know if you need more details or a different test case! |
- Merge upstream main and resolve conflicts in identifier_quote_test.go - Improve hardcoded logic in ensureCheckClauseParens using depth-based validation - Remove duplicate quoteIdentifierIfNeeded function, use util.QuoteIdentifier instead - Fix CHECK constraint parsing for domains and expressions - Add parentheses around comparison operators to match PostgreSQL's internal format - Fix unused import issue All tests now pass (100% success rate). Addresses maintainer's feedback about hardcoded logic and resolves merge conflicts.
screenfluent
force-pushed
the
fix/check-between-parsing
branch
from
66b5a6e to
3f739f4
Compare
|
@tianzhou conflicts resolved and hardcoded issues fixed. Using depth-based validation now instead of hardcoded checks. |
Fix identifier quoting in CHECK constraints and BETWEEN parsing
Problems Fixed
This PR addresses two critical issues with CHECK constraint parsing that affect schemas using camelCase naming (common in Better Auth, Prisma, and other modern ORMs):
1. CHECK constraints losing identifier quoting
Before:
After:
-- Output: CHECK ("followerCount" >= 0) -- Correct: preserves quotes2. BETWEEN incorrectly parsed
Before:
After:
-- Output: CHECK ("stockLevel" BETWEEN 0 AND 1000) -- Correct: proper AND syntaxChanges
internal/ir/parser.goquoteIdentifierIfNeeded()(~15 lines) to preserve camelCase identifiersextractColumnName()to quote identifiers when neededparseAExpr()to handle BETWEEN with properX AND Ysyntax (not tuple)wrapInParens()helper to ensure correct parentheses in CHECK expressionsinternal/diff/table.goensureCheckClauseParens()as defensive measure in SQL generationTests
internal/diff/identifier_quote_test.goImpact
Testing
Note on Code Duplication
The
quoteIdentifierIfNeeded()function introduces minor duplication with existing quoting logic in the diff package. This is intentional to keep the fix minimal and focused. A follow-up PR can extract this to a shared utility package to maintain DRY principles.Related Issues